Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bubble sort #26

Closed
wants to merge 3 commits into from
Closed

bubble sort #26

wants to merge 3 commits into from

Conversation

Shree7676
Copy link
Contributor

just added bubble sort algo.

@seberg
Copy link
Collaborator

seberg commented Aug 27, 2024

This looks like a great start. Unfortunately, we also need a test for this. You should find a small test stub.

Copy link
Collaborator

@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good with the tests (covering some maybe interesting paths like []).
For list inputs there may still be a subtlety about returning a copy or mutating the list in place.

Anyway, besides documentation, I think this is good!

def bubble_sort(l):
# We should provide bubble sort as well!
raise NotImplementedError
def bubble_sort(arr):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need some documentation of course. At least for public facing functions.

# Test even-sized list:
l = [3, 2, 1, 5, 4, 6]
res = lws.bubble_sort(l)
assert res == [1, 2, 3, 4, 5, 6]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list of tests is good. One way one can write it more compact is for example pytest.mark.parametrize().
That way the input and expected results are parameter, and the repetitive code is a bit less (not much for the simple test). It also effectively splits it in multiple tests.

@Shree7676
Copy link
Contributor Author

creating a new pull request with all the changes and closing this one

@Shree7676 Shree7676 closed this Aug 29, 2024
@seberg
Copy link
Collaborator

seberg commented Aug 29, 2024

You should be able to force-push, but OK :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants